Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix tests that fail on macOS 14 #5441

Merged
merged 5 commits into from
Nov 22, 2023
Merged

Fix tests that fail on macOS 14 #5441

merged 5 commits into from
Nov 22, 2023

Conversation

dlon
Copy link
Member

@dlon dlon commented Nov 15, 2023

Main changes:

  • Fix potential races in the management interface by returning from the following commands only after the tunnel state has been updated: set_allow_lan, set_dns_options, set_block_when_disconnected.
  • Obtain tunnel interface from management interface instead of hardcoding it. This was too fragile, particularly on macOS.
  • Work around bug involving fetch-tags in the GHA e2e workflow. Use git fetch --tags directly.
  • Simplify test_multihop and test_udp2tcp (analogously to Remedy connection check timeouts in test_bridge #5442).

Fix DES-443.


This change is Reviewable

@dlon dlon force-pushed the fix-macos-tests branch 3 times, most recently from b1b5c2f to c2fc533 Compare November 16, 2023 08:31
@dlon dlon marked this pull request as ready for review November 17, 2023 09:44
@dlon dlon force-pushed the fix-macos-tests branch 3 times, most recently from b912a87 to 33bba9e Compare November 19, 2023 13:23
Copy link

linear bot commented Nov 20, 2023

DES-443 Fix failing tests on macOS 14

Most tests currently fail on macOS 14. David mentioned it might be because the test framework is picking the wrong network interface for something.

@dlon dlon force-pushed the fix-macos-tests branch 6 times, most recently from ab224fb to f79a0aa Compare November 20, 2023 17:18
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, looking forward to getting this merged! 🎉

Reviewed 21 of 21 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)


test/test-manager/src/tests/helpers.rs line 99 at r1 (raw file):

pub async fn send_guest_probes(
    rpc: ServiceClient,
    interface: Option<String>,

This argument does not have to be an Option<String>, it could safely be just a String.
I did not find any instance where None was passed as the interface argument.

Code quote:

interface: Option<String>

test/test-runner/src/net.rs line 56 at r1 (raw file):

    log::debug!("Connecting from {bind_addr} to {destination}/TCP");

    tokio::task::spawn_blocking(move || {

What is the motivation for this change?
I suggest reverting this change and use the tokio::net::TcpStream, instead of manually putting synchronous I/O on a blocking-friendly thread.

Code quote:

tokio::task::spawn_blocking(move || {

test/test-runner/src/net.rs line 121 at r1 (raw file):

    }

    let _ = tokio::task::spawn_blocking(move || {

dito

Code quote:

 let _ = tokio::task::spawn_blocking(move || {

Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


test/test-runner/src/net.rs line 56 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

What is the motivation for this change?
I suggest reverting this change and use the tokio::net::TcpStream, instead of manually putting synchronous I/O on a blocking-friendly thread.

connect() sometimes fails due to EINPROGRESS. I'm not sure whether it's fine to convert the socket to a tokio::net::TcpStream in this state. I suppose we could use blocking I/O only for connect().

Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


test/test-manager/src/tests/helpers.rs line 99 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

This argument does not have to be an Option<String>, it could safely be just a String.
I did not find any instance where None was passed as the interface argument.

We're calling send_guest_probes_without_monitor in install.rs. Should this function not have the same interface as that one?

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)


test/test-manager/src/tests/helpers.rs line 99 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

We're calling send_guest_probes_without_monitor in install.rs. Should this function not have the same interface as that one?

I think a function's interface should reflect what it actually needs - in this case I find it a bit leaky to have send_guest_probes also take interface as an Optional<String> just because it calls send_guest_probes_without_monitor 😊


test/test-runner/src/net.rs line 56 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

connect() sometimes fails due to EINPROGRESS. I'm not sure whether it's fine to convert the socket to a tokio::net::TcpStream in this state. I suppose we could use blocking I/O only for connect().

Alright, nevermind then!

Copy link
Member Author

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 18 of 21 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


test/test-manager/src/tests/helpers.rs line 99 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I think a function's interface should reflect what it actually needs - in this case I find it a bit leaky to have send_guest_probes also take interface as an Optional<String> just because it calls send_guest_probes_without_monitor 😊

Done.

Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 2 of 3 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@dlon dlon merged commit f7eb43c into main Nov 22, 2023
27 of 28 checks passed
@dlon dlon deleted the fix-macos-tests branch November 22, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants